Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log file path when image open throws exception #13144

Merged
merged 1 commit into from
Jan 24, 2018

Conversation

pmclain
Copy link
Contributor

@pmclain pmclain commented Jan 11, 2018

Description

Log the offending file when opening an image throws an exception. The current exception message does a good job describing the problem, but does not tell you which file caused the issue. The proposed change would alter the log entry from:
Unsupported image format.
to:
Unsupported image format.: /path/to/my/bad/file.png

Fixed Issues (if relevant)

none

Manual testing scenarios

  1. Point product small_image to a corrupt/unsupported image.
  2. Navigate to product list page containing product from step 1. The page should partially render, likely without any products.
  3. View error report in /var/report to see the path to problem image.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@orlangur orlangur self-assigned this Jan 11, 2018
try {
$this->_adapter->open($this->_fileName);
} catch (\Exception $e) {
throw new \Exception("{$e->getMessage()}: {$this->_fileName}");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not add such information directly in adapter->open() method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it doesn't need to be added to both existing adapters and any future adapters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit odd to catch exception and throw immediately to add information which was passed to adapter method anyway. Please move this addition to adapter(s) as it's clearly his responsibility to throw such exception.

As always, please do commit amend and force push.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orlangur ready for another look.

@TomashKhamlai
Copy link
Contributor

Hello, @pmclain! I am testing the changes that come with this PR and I was not able to reproduce the issue. Could you describe how to get the image that cause the error or just post it here in zip archive? I tried TIFF images and images that are corrupted with HEX-editor but with no success.

Here is what I tried:
question

@pmclain
Copy link
Contributor Author

pmclain commented Jan 16, 2018

@TomashKhamlai I'm attaching one of the corrupt images that have been giving me trouble. I wasn't able to upload this via the admin panel. I am primarily seeing this when running imports using external URIs for images, and I have been unable to reproduce reliably enough to determine if the download is failing or needing a more reliable validation mechanism. You may need to assign the product image attribute directly in the database or replace an existing image with a corrupt image, like the attached.
corrupt_image.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants